-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
362 add and edit attributes in sample types #2032
base: main
Are you sure you want to change the base?
362 add and edit attributes in sample types #2032
Conversation
add attribute button is still visible, even when the sample type has samples.
Pass a Hash describing wich attributes have a different title.
User should only be able to change the sample type attribute's title if the user has editing permission to all samples.
Add explaination to why changing the attribute's name is prohibited
Add new scenario's where tests should fail or succeed
More readable
Effect of adding `samples.all?(&:can_edit?)` clause ini editing constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagged a few issues, mostly around concurrency
queue_as QueueNames::SAMPLES | ||
|
||
def perform(sample_type, attribute_changes = [], user) | ||
Seek::Samples::SampleMetadataUpdater.new(sample_type, attribute_changes, user).update_metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question, but what happens if someone views a sample before the job has finished running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On your local instance you could update a sample type without any workers running. Alternatively, add a sleep
here or in the job.
|
||
after_action :update_sample_json_metadata, only: :update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if 2 updates were made to a sample type in a short time?
I think you may need a mechanism for "locking" the sample type to further changes until the update job has completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that was possible. Has it already been done somewhere else in the source code?
metadata = JSON.parse(sample.json_metadata) | ||
# Update the metadata keys with the new attribute titles | ||
@attribute_change_maps.each do |change| | ||
metadata[change[:new_title]] = metadata.delete(change[:old_title]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone creates a new sample before the job finishes, this line will wipe out the changed attributes.
Closes #1257
Closes #1258
Closes #362